fix(dry-run): thread { dryRun } into the queue lock, so a preview stops creating <home>/.teamai/locks/ - #896
Conversation
…tes nothing `--dry-run` promises no changes made. On a fresh self-mode clone one write still gets through: an empty `<home>/.teamai/locks/` is created and left behind. It is only the directory, not a lock file, but it is a real filesystem change and the suite can see it -- `dry-run-load-path.test.ts` declares it as a tolerated entry for `pull` today, which is the honest way of saying the preview is not clean. The mechanism is already on main: Tencent#866 gave `acquireLock` an `options.dryRun` that reads the lock state instead of taking it, and `pull.ts:1873`, `push.ts:784` and `push.ts:839` pass it. The queue lock does not: learnings-publish.ts:75 syncLock === null || await acquireLock(syncLock) <- not passed learnings-publish.ts:93 listPendingForInstall(localConfig) <- not passed pending-learnings.ts:77 withQueueLock(...) -> acquireQueueLock(home) <- not passed pending-learnings.ts:66 if (await acquireLock(lockPath)) <- takes the real lock `pull` counts the queue so it can report how many learnings it would publish, and counting takes the queue lock, because the queue owns it. Taking that lock is a write: `acquireLock` ensures the lock parent (`update.ts:404`) and `releaseLock` removes the lock FILE but not that directory (`update.ts:464`), so the directory outlives the command. This threads the flag down. Three signatures gain an optional `{ dryRun?: boolean } = {}` and forward it; existing callers (`migrate.ts:413`, `migrate.ts:891`, and the `withQueueLock` inside `savePendingLearning`) send `{ dryRun: undefined }` and are unchanged. `readPendingForInstall` deliberately does not get the parameter: its only caller is behind `if (locked && !dryRun)` (`learnings-publish.ts:94`), so a preview never reaches it. The test gets stricter -- `pull`'s allowed list goes from `[PULL_LOCK_DIR]` to `[]`. `snapshotTree` records directories as `"<rel>/" = 'dir'`, so an empty directory is counted; that is why the entry had to be written at all. Verification: - before: `vitest run src/__tests__/dry-run-load-path.test.ts` -> 22 passed - after: same -> 22 passed, with `pull`'s allowed list empty - mutation: reverting `acquireLock(lockPath, options)` to `acquireLock(lockPath)` turns the case red and names the cause, "appeared": ["home\\.teamai\\locks/"] - no collateral: `lock-atomic` 22/22, `pull-post-checks` 13/13, `pull-placement-reconcile` 2/2; `tsc --noEmit` rc=0 and `oxlint --deny-warnings` rc=0 - the locally-failing files (`pending-learnings`, `git-kind-learnings`, `checkout-refusal-silent`) fail identically on the unmodified base: POSIX path assertions on Windows and EBUSY from git-worktree fixtures in workers
|
|
The In one line: CI on this head is green: 12 checks pass, the single skip being If a fresh automated pass would help, that reviewer re-runs on a push or on a draft→ready transition — and this PR already carries a maintainer as assignee, so its authorization gate would pass. Happy to toggle it if you want one. |
--dry-runpromises no changes made. On a fresh self-mode clone one write still slips through:an empty
<home>/.teamai/locks/is created and left behind.It is only the directory — no partition, no lock file — but it is a real change to the filesystem and
the suite can see it:
dry-run-load-path.test.tsdeclares that directory as a tolerated entry forpulltoday. Tolerated is the honest word for it; the preview is not clean.This is #866's leftover, and it is small — the mechanism is already on
main, the call sites justnever passed the argument.
The mechanism exists
acquireLockgrewoptions.dryRunin #866. A preview does not take the lock; it returns the verdicta real run would get:
pull.ts:1873,push.ts:784andpush.ts:839all pass it. The queue lock does not:pullonly wants to report how many queued learnings it would publish, so it lists the queue — andlisting takes the queue lock, because the queue owns it. Taking that lock is a write.
releaseLockthen removes the lock file but not the directory it was created in, so the empty directory stays.
The same gap sits one frame up:
learnings-publish.ts:75callsacquireLock(syncLock)with no optionseither.
pullshort-circuits it withholdsSyncLock: true, but any caller that does not passholdsSyncLockstill takes a real sync lock under a preview and creates its parent directory.The fix
Thread
dryRundown the chain. Three signatures gain an optional{ dryRun?: boolean } = {}andforward it; the single
acquireLockthey reach stops writing.Every new parameter is optional and defaults to
{}, so a caller that passes nothing sends{ dryRun: undefined },if (options.dryRun)is false, and behaviour is unchanged —migrate.ts:413and
migrate.ts:891callacquireQueueLock/withQueueLockand are untouched, as is thewithQueueLockinsidesavePendingLearning.readPendingForInstalldeliberately does not get the parameter. Its only caller(
learnings-publish.ts:241, insidequeueImportRemnants) sits behindif (locked && !dryRun)atlearnings-publish.ts:94, so it is not reachable under a preview and a flag would be dead weight.The test gets stricter
dry-run-load-path.test.tscurrently declares the directory as a tolerated entry forpull:[]is the point of #866 and now holds forpulltoo: the assertion isexpect({ appeared, vanished }).toEqual({ appeared: allowed, vanished: [] }), andsnapshotTreerecords directories as
"<rel>/" = 'dir', so an empty directory is counted. If the directory comesback, this case fails — the entry was not decorative, and removing it is the actual claim.
Verified
An end-to-end record is required for a runtime-behavior change (
AGENTS.md→## Code Review Rules),so here is one, plus the control that makes it mean something.
Unit —
vitest run src/__tests__/dry-run-load-path.test.tspull's entry tolerated.pull's allowed list empty.acquireLock(lockPath, options)→acquireLock(lockPath)turns thecase red, and it names the cause:
"appeared": ["home\\.teamai\\locks/"].Real CLI —
node dist/index.js --dry-run pull, against a live git fixture--dry-runis a global option (src/index.ts:79) andpull's action spreadsglobalOpts, so theinvocation is
teamai --dry-run pull—teamai pull --dry-runis not accepted.The fixture is the shape the defect needs and nothing else: a fresh self-mode clone, where the partition
does not exist yet. It mirrors
setupSelfModeCloneindry-run-load-path.test.ts—app/.teamai/teamai.yamlwith
mode: self,app/.teamai/manifest/roles.yaml, a git repo with anoriginremote, andHOMEpointing at an otherwise empty
home/carrying.teamai/and.claude/. The whole tree is hashed beforeand after (directories included, recorded as
<rel>/), so an empty directory counts as a write.home/.teamai/locks/home/.teamai/debug.logacquireLock(lockPath, options)reverted toacquireLock(lockPath)home/.teamai/debug.log,home/.teamai/locks/The second row is the point of the first. Because the control is built from this diff with one line
reverted, the directory appearing there is attributable to that line — and it also rules out the reading
that "no
locks/" means the preview never got that far. It did get that far: the same run reportsproject scope detectedand[project] Team repo: single-repo (knowledge on main), i.e. it reaches theproject scope, which is where
publishQueuedLearnings→listPendingForInstall→acquireQueueLocksits.debug.logis the only other new entry and it is not this change's: it is the logger's own file, writtenby every command, and it appears in both rows. Nothing that already existed was rewritten or removed in
either run. The run performs no fetch against its placeholder remote — the debug log shows local work only.
The driver is a local script, not added to this PR: the repo's own
vitest.e2e.config.tssite alreadyspawns
dist/index.js, which is what CI'sE2E (fork-safe, no credentials)job runs. Say the word and Iwill land the driver as
scripts/dry-run-e2e.mjsnext toscripts/model-routes-e2e.mjs, which is theexisting precedent for a standalone real-CLI check.
Static —
tsc --noEmitrc=0;oxlint --deny-warnings --report-unused-disable-directivesrc=0.Locally red, and not attributed to this change —
pending-learnings,git-kind-learningsandcheckout-refusal-silentfail identically on the unmodified base: POSIX-hardcoded path assertions onWindows (
\home\user\...vs/home/user/...), andEBUSYfrom git-worktree fixtures inside vitestworkers. Established by running each file on the unmodified base, not by inspection.
Not touched, on purpose
releaseLockstill removes the lock file but not the directory it was created in. Making it symmetricis tempting and is the wrong end: the comment above
acquireLockalready states the intended answer —a preview does not take the lock — and a real run creating
<home>/.teamai/locks/is correct, thatdirectory is where locks live.
releaseLockalso has callers that pass a path whose parent existedfirst (
bootstrap.ts:233,models/switch.ts:66,dashboard-collector.ts:1540); removing a parentthere would let two concurrent commands delete each other's lock directory.